-
Notifications
You must be signed in to change notification settings - Fork 3
feat/use new multiscales #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat/use new multiscales #75
Conversation
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| "scale_factor": 2**level, | ||
| "translation_relative": 0.0, | ||
| "scale_absolute": 1.0, | ||
| "scale_relative": 2**level, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we imaging other base scale factors than 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added `initialize_crs_from_dataset` function to extract CRS from dataset metadata. - Updated S2 optimization commands to include new CRS handling. - Removed unused arguments related to geometry and meteorology groups. - Added comprehensive tests for CRS initialization from various sources.
emmanuelmathot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
besides my addings for a better support of CRS after CPM 2.6.0, this is great!
I validated with a test in titiler-eopf
| # verify exact output group structure | ||
| # this is a sensitive, brittle check | ||
| expected_structure_json = tuplify_json( | ||
| json.loads( | ||
| ( | ||
| Path("tests/test_data_api/geozarr_examples/") | ||
| / (s2_group_example.stem + ".json") | ||
| ).read_text() | ||
| ) | ||
|
|
||
| # Save as zarr | ||
| ds.to_zarr(test_input, zarr_format=3) | ||
| ds.close() | ||
|
|
||
| # Test CLI with --crs-groups but no groups specified | ||
| cmd = [ | ||
| ) | ||
| observed_structure_json = tuplify_json( | ||
| GroupSpec.from_zarr(zarr.open_group(output_path)).model_dump() | ||
| ) | ||
| assert expected_structure_json == observed_structure_json, view_json_diff( | ||
| expected_structure_json, observed_structure_json | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emmanuelmathot this is an important check -- we are very strictly checking the structure of the zarr group against a reference JSON document. Any time we change the output format, this will fail, which is a sign that we need to update the JSON files that represent the expected Zarr group structure
| @@ -0,0 +1,16098 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emmanuelmathot sorry for the number of lines here but this JSON document is used to check the output of the s2 optimized conversion. that means whenever we change the output of the conversion, we need to update this document. I think we can rely on better checks in the future.
| @@ -0,0 +1,16098 @@ | |||
| { | |||
| "attributes": {}, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we do not export some of the top-level attributes from the original data, such as other_metadata:
Line 3 in 41c3490
| "attributes": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include them indeed. This is a bug from the original version and an unexpected behavior from xarray
|
I am still performing validation tests but first ones are ok. |
This is an extensive, but still first-order, refactorization of the optimized s2 multiscales conversion to use the new multiscales metadata.
A few key changes:
S2OptimizedConverter,S2MultiscalePyramid, andS2OptimizationValidatorclasses have been broken out into functions.